Skip to content

test(formula): correct the stale #6133 comment in parse-cel-to-ast.test.ts (#6678) - #7203

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-6678-stale-defect-comment
Aug 10, 2026
Merged

test(formula): correct the stale #6133 comment in parse-cel-to-ast.test.ts (#6678)#7203
os-zhuang merged 1 commit into
mainfrom
claude/issue-6678-stale-defect-comment

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Closes #6678.

packages/formula/src/parse-cel-to-ast.test.ts carried two comments describing classifyError's pre-#6202 behaviour as if it were current. The first does not merely go stale, it instructs: it tells the next author that asserting ((record.a) -> parse "would enshrine it", i.e. that today's correct verdict is a bug not to be pinned.

What I measured, before rewriting anything

Per the card, the current grading was established by running the case, not by paraphrasing PR #6202. Temporary probe on this branch calling celEngine.compile over the whole SYNTAX_REJECTED list (probe removed before the commit):

MEASURED | "((record.a)"     -> kind=parse | message[0]="Expected RPAREN, got EOF"
MEASURED | "record.budget >" -> kind=parse | message[0]="Unexpected token: EOF"
MEASURED | "record.a $$ 1"   -> kind=parse | message[0]="Unexpected character: $"
MEASURED | "record.a ?? 3"   -> kind=parse | message[0]="Unexpected token: QUESTION"

 Test Files  1 passed (1)
      Tests  4 passed (4)

Two things fall out of that, and only one of them was in the card:

  1. The grading claim in the comment is false — ((record.a) grades parse, not runtime.
  2. The wording claim in the same comment is still true — cel-js does still phrase it Expected RPAREN, got EOF. What changed is that the phrasing no longer decides anything: classifyCelFault now branches on err instanceof ParseError (celEngine 的 classifyError 把「括号/方括号不配对」这类真语法错答成 kind: 'runtime',而该 kind 会原样出现在作者可见的拒写消息与 REST 响应体里 #6133 / PR fix(formula): classifyError 把 cel-js 的 parse 错误按错误类归为 syntax/parse,不再误报 runtime (#6133) #6202), and the keyword table was deleted outright by classifyError 的 type/runtime 两支仍按文案分类,而 cel-js 把作者源码嵌进 message —— 字段名叫 parse_status 的记录,求值期故障被答成 kind: 'parse' #6223 / PR fix(formula): grade CEL faults by error class + code, not by the message (#6223) #6677. The rewritten comment keeps the measured wording and corrects the mechanism, rather than deleting both.

Confirming the regex the comment quotes is gone: the only occurrence of parse|unexpected|syntax anywhere under packages/formula/src/ was the stale comment itself.

Were the surrounding assertions stale too?

No — checked, and this is the one part of the card that resolves in the reassuring direction. Nothing in the file asserts runtime for any source; the only runtime on line 142 was inside the prose. The old grading was never pinned, it was omitted — the test asserted record.budget > and deliberately said nothing about ((record.a). So there is no second half to file: the fix is text plus closing the omission the text was rationalising.

Changes

  • :115-121 — the parity note now says classification is independent of wording and cites the two PRs that made it so.
  • :135-144 — the "NOT asserted / would enshrine it" note is replaced by an it.each that asserts both wordings grade parse, promoting ((record.a) to an ordinary assertion now that its verdict is stable. The comment records both measured messages and points at cel-error-classification.test.ts as the exhaustive per-wording matrix, so the two suites do not read as rival sources of truth.

Changeset

None added; this PR needs the skip-changeset label. Read from the gates rather than assumed:

Worth noting for the lane: taking that label means changeset-check is skipped wholesale, so the changeset-family self-tests do not run from there. They are covered anyway — lint.yml's Changeset-family gate self-tests step is deliberately unconditional (#6509).

Verification

pnpm --filter @objectstack/formula exec vitest run \
  src/parse-cel-to-ast.test.ts src/cel-error-classification.test.ts

 Test Files  2 passed (2)
      Tests  82 passed (82)

Related: #6678, #6133 / PR #6202, #6223 / PR #6677, #4812.


Generated by Claude Code

…st.ts (#6678)

Two comments in `parse-cel-to-ast.test.ts` described `classifyError`'s
pre-#6202 behaviour as if it were current:

- `:139-143` told the next reader that `((record.a)` is graded `runtime`
  because cel-js's `Expected RPAREN, got EOF` misses `classifyError`'s
  /parse|unexpected|syntax/i, and instructed them that asserting the
  correct verdict "would enshrine it".
- `:116-118` gave "`classifyError` reads the phrasing" as the live reason
  for a test-design decision.

Neither holds. PR #6202 (#6133) made the ParseError arm structural and
PR #6677 (#6223) deleted the keyword table; the regex quoted in the first
comment had no remaining occurrence in the package except that comment.

Measured on this branch rather than taken from the changelog:

    "((record.a)"     -> kind=parse | Expected RPAREN, got EOF
    "record.budget >" -> kind=parse | Unexpected token: EOF
    "record.a $$ 1"   -> kind=parse | Unexpected character: $
    "record.a ?? 3"   -> kind=parse | Unexpected token: QUESTION

The message wording is unchanged; only the grading moved. The comments
are rewritten to that measurement, and `((record.a)` is promoted from
"deliberately not asserted" to an ordinary assertion alongside
`record.budget >`, since the verdict it was withheld over is now stable.

No assertion in the file encoded the old grading, so nothing behavioural
was pinned wrong; the omission was the only stale artefact besides prose.
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 10, 2026 2:37am

Request Review

@os-zhuang os-zhuang added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 10, 2026 — with Claude
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 0 changed package(s). ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

1 participant